@maxime-rainville That's correct, the goal is to have smarter "dirty" state and also allow some level of configuration.

From memory, there's a ChangeSet object which is used by publishRecursive. My main concern at this stage is that this PR might be duplicating the ChangeSet logic and/or diverging from it in subtle ways that will create ambiguity down the road. It's probably worth asking ourselves if there's a way we could have some sort of unified logic for both.

@silverstripeux Currently, if you have an object that owns a bunch of child object with unpublished changes, the CMS doesn't show that object in a "dirty" state. If this PR got merge, we would be very close to being able to highlight such a scenario. How much value would you put on this kind of improvement?

e.g.: You have a UserForm page. The UserForm page has no outstanding changes directly applied to it, but one of its form field has been modified. So logically, the UserForm page should be in a dirty state as well.

@maxime-rainville it does trip CMS users up, they think their page is published and nothing needs to be done further when in-fact deeper content isn't reflected on the published page, hence I would put a lot of value on this. We don't have the ability to train users about this stuff so rely on UI experiences to inform people.

@brynwhyman I had a preliminary review. Looks like this has the potential to provide substantial benefit beyond Mojmir's use case.

My main concern would be making sure the chosen approach aligns with the existing logic in ChangeSet.

@maxime-rainville Added an interface for the service as requested.

Just want to cc @chillu into this pull request too. This sounds like it might overlap some of the work that was done with versioned-snapshots to better understand the state of child objects - admittedly that had more of a focus on the version records, but still...

As an aside, would this PR also resolve this issue? silverstripe/silverstripe-elemental#790

@brynwhyman Just to provide some context:

Version snapshots it essentially a logger of events, the main drive for this module was to bypass issues with Fluent / Versioned (which are now resolved in Fluent 6), it doesn't provide you with information about state of objects which is the goal of this PR. I don't think there is an overlap here.

The linked Elemental PR is a subcase of the issue which this PR is trying to solve. Note that usage of this feature is also relevant for models outside of blocks.

Alright, thanks @mfendeksilverstripe that's helpful 😄

Does this need more reviewing? I can help out. In the meantime I've patched this with an extension, but this seems much more robust.

FWIW This has been flagged as a bug by clients of ours

@emteknetnz @maxime-rainville This is ready for a review.

As this is adding new functionality, please target this to the 2 branch, rebase against that branch, any resolve and conflicts that might arise.

Hey @maxime-rainville I would like to get some clarity on what to do with this PR. @GuySartorelli provided me with very good feedback which I'm happy to action. However, from my past experience these PRs even after sinking a lot of effort into them and addressing all the feedback might not get merged because of lack of alignment on how we actually want to approach this problem.

This is my understanding of the situation:

• we agree that we have a systemic issue with how page changes are detected in the CMS as the button states don't really work with nested owned models (such as content blocks)
• this feature is really important to content authors as it prompts them to publish changes
• there are also other components that are impacted by this, for example Cancel draft changes UI component doesn't show up for draft changes on owned models silverstripe-cms#2892

From the feedback I see two distinct groups of changes:

• long term solution which redefines how ownership is managed - this is CMS 6 stuff the earliest
• workaround solution for CMS 4 / 5 - something that we can deliver now

Please let me know which of these (if any) should this PR focus on.

Hey @emteknetnz This change is aiming to address the issues related to initial page state only. Yes, we would need to update several places in the CMS UI where stagesDiffer() is used and replace it with stagesDifferRecursive() to achieve that. Most notable places would be the publish button state but also other components such as Cancel draft changes action.

I recognise that there are other issues with the publish button state and cancel draft changes action which are caused by user interaction. Most notable case is the usage of block level edit in the elemental inline editor. These issues are out of scope of this PR though.

Cancel draft changes action

What's a cancel draft changes action?

What's a cancel draft changes action?

@emteknetnz Please check on silverstripe/silverstripe-cms#2892 , I left more details there.

OK cool thanks, so it's intended use case is limited to a couple of things that are both calculated in SiteTree::getCMSActions(). Hopefully should be easy to rollout then

From the feedback I see two distinct groups of changes:
long term solution which redefines how ownership is managed - this is CMS 6 stuff the earliest
workaround solution for CMS 4 / 5 - something that we can deliver now

I think treating this PR as a "workaround" PR is fine, IMO it's not really a workaround it's almost a bugfix to get the existing architecture (which was probably designed pre-elemental) to work as it should. My only real concern is that there will be a performance hit on this, particularly if you have a large number of blocks. I'm not sure how impactful this would be in practice though, it may be negligible.

I'd agree with Guy that this should just go straight into Versioned rather than having a separate service class. While I get that the intention is to split things off in to easily digestable pieces, it just make it things inconsistent because all the existing stuff in Versioned just stays where it is. I'd much rather just keep adding to Versioned for now.

Agreed to remove the cascade_duplicates based traversal - we will only use owns which is consistent with publishRecursive()

• add test for versioned model > non-versioned model > versioned model

@GuySartorelli I've pushed up new changes as per our discussion, please review.

cc: @maxime-rainville

Hey @GuySartorelli I believe I addressed all the feedback from you. Please hit me up if there are more things to follow up on 😉

Just the one requested change - then I'll be happy to merge.